[management] Move service reload outside transaction in account settings update#5325
Merged
[management] Move service reload outside transaction in account settings update#5325
Conversation
Signed-off-by: bcmmbaga <bethuelmbaga12@gmail.com>
Contributor
📝 WalkthroughWalkthroughDefers reverse-proxy reloads during account updates by introducing a local Changes
Sequence Diagram(s)sequenceDiagram
participant API as Account API
participant DB as Database
participant Event as Event Dispatcher
participant RP as ReverseProxyManager
participant Peers as PeerManager
API->>DB: Begin account update transaction
DB-->>API: Ack
API->>DB: Update network fields (e.g., NetworkRange)
DB-->>API: Ack
API->>API: set reloadReverseProxy = true
API->>DB: Commit transaction
DB-->>API: Commit ack
API->>Event: emit account update events (DNS/network)
Event->>RP: notify of network/DNS change
RP->>RP: ReloadAllServicesForAccount (performed after tx)
Note right of RP: previously would call\nPeers.UpdateAccountPeers() here (removed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@management/server/account.go`:
- Around line 396-404: The guard using reloadReverseProxy prevents
UpdateAccountPeers from running when
reverseProxyManager.ReloadAllServicesForAccount(ctx, accountID) fails; change
the logic in the block around ReloadAllServicesForAccount so that on error you
reset reloadReverseProxy to false (or otherwise ensure the fallback path runs) —
i.e., after calling am.reverseProxyManager.ReloadAllServicesForAccount(...), if
err != nil log the error as before and set reloadReverseProxy = false so the
subsequent if !reloadReverseProxy && (updateAccountPeers || extraSettingsChanged
|| groupChangesAffectPeers) will invoke am.UpdateAccountPeers(ctx, accountID).
Ensure you only flip the flag on failure and keep successful reloads unchanged.
- Around line 396-400: The call to
am.reverseProxyManager.ReloadAllServicesForAccount(...) in UpdateAccountSettings
is not nil-safe; add a defensive nil-check around am.reverseProxyManager (e.g.,
if am.reverseProxyManager != nil { ... }) before calling
ReloadAllServicesForAccount, or alternatively document/validate that
SetServiceManager must be called during initialization so reverseProxyManager is
guaranteed non-nil; prefer adding the nil guard to make UpdateAccountSettings
robust in tests and alternative init flows.
Comment on lines
+396
to
+400
| if reloadReverseProxy { | ||
| if err = am.reverseProxyManager.ReloadAllServicesForAccount(ctx, accountID); err != nil { | ||
| log.WithContext(ctx).Warnf("failed to reload all services for account %s: %v", accountID, err) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how reverseProxyManager / SetServiceManager is initialized and where it's called
echo "=== SetServiceManager call sites ==="
rg -n 'SetServiceManager' --type go -C 3
echo ""
echo "=== reverseProxyManager usage ==="
rg -n 'reverseProxyManager' --type go -C 2
echo ""
echo "=== ReloadAllServicesForAccount interface definition ==="
ast-grep --pattern $'type Manager interface {
$$$
ReloadAllServicesForAccount($$$) $$$
$$$
}'Repository: netbirdio/netbird
Length of output: 29813
🏁 Script executed:
#!/bin/bash
# Check the initialization lifecycle and AfterInit pattern
echo "=== AfterInit callback pattern in modules.go ==="
sed -n '100,115p' management/internals/server/modules.go
echo ""
echo "=== BuildManager function (account manager creation) ==="
sed -n '95,130p' management/internals/server/modules.go
echo ""
echo "=== UpdateAccountSettings function signature ==="
rg -n 'func.*UpdateAccountSettings' --type go -A 2
echo ""
echo "=== How UpdateAccountSettings is typically called (checking for direct calls) ==="
rg -n 'UpdateAccountSettings' --type go | grep -v '_test.go' | head -20Repository: netbirdio/netbird
Length of output: 5963
🏁 Script executed:
#!/bin/bash
# Check the BaseServer initialization lifecycle and AfterInit timing
echo "=== BaseServer structure and AfterInit definition ==="
rg -n 'type BaseServer' --type go -A 20 | head -40
echo ""
echo "=== AfterInit method implementation ==="
rg -n 'func.*AfterInit' --type go -B 2 -A 10
echo ""
echo "=== Server startup sequence - when handlers/HTTP listeners start ==="
rg -n 'func.*Start\|func.*Run' management/internals/server/base_server.go --type go -B 2 -A 5 | head -60
echo ""
echo "=== Check accounts_handler.go context and registration ==="
sed -n '1,50p' management/server/http/handlers/accounts/accounts_handler.goRepository: netbirdio/netbird
Length of output: 4607
🏁 Script executed:
#!/bin/bash
# Check when afterInit callbacks are executed in Start()
echo "=== Full Start() method ==="
sed -n '92,180p' management/internals/server/server.go
echo ""
echo "=== HTTP routing/handler setup in Start() ==="
sed -n '92,250p' management/internals/server/server.go | tail -100Repository: netbirdio/netbird
Length of output: 6922
🏁 Script executed:
#!/bin/bash
# Verify the initialization order guarantee more explicitly
echo "=== APIHandler creation in Start() ==="
rg -n 'APIHandler\(\)' management/internals/server/server.go -B 5 -A 5
echo ""
echo "=== When APIHandler is created and when HTTP listener starts accepting requests ==="
sed -n '150,170p' management/internals/server/server.goRepository: netbirdio/netbird
Length of output: 1460
Add nil guard or document the initialization guarantee for reverseProxyManager.
The code at lines 396–400 calls am.reverseProxyManager.ReloadAllServicesForAccount() without a nil check. While the server initialization framework guarantees that SetServiceManager is called via AfterInit before the HTTP listener starts accepting requests (and thus before UpdateAccountSettings can be invoked), the lack of defensive nil checks creates fragility if the code is refactored, reused in different initialization contexts, or in test scenarios. Adding a nil guard (e.g., if am.reverseProxyManager != nil { ... }) would improve robustness, particularly given that other code paths in the codebase also use reverseProxyManager without similar guards.
🤖 Prompt for AI Agents
In `@management/server/account.go` around lines 396 - 400, The call to
am.reverseProxyManager.ReloadAllServicesForAccount(...) in UpdateAccountSettings
is not nil-safe; add a defensive nil-check around am.reverseProxyManager (e.g.,
if am.reverseProxyManager != nil { ... }) before calling
ReloadAllServicesForAccount, or alternatively document/validate that
SetServiceManager must be called during initialization so reverseProxyManager is
guaranteed non-nil; prefer adding the nil guard to make UpdateAccountSettings
robust in tests and alternative init flows.
Signed-off-by: bcmmbaga <bethuelmbaga12@gmail.com>
|
mlsmaycon
approved these changes
Feb 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Issue ticket number and link
fixes #5318
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Bug Fixes
Chores
Tests